CLN: #1836 merge pyrefly_strict into pyrefly, make ty_all and pyrefly_all passing - #1867
CLN: #1836 merge pyrefly_strict into pyrefly, make ty_all and pyrefly_all passing#1867cmp0xff wants to merge 47 commits into
pyrefly_strict into pyrefly, make ty_all and pyrefly_all passing#1867Conversation
2232058 to
461372e
Compare
0122fa5 to
2770692
Compare
497906a to
c3e92f5
Compare
c3e92f5 to
bd902dc
Compare
642ad36 to
24d0eff
Compare
d661339 to
51fbbbc
Compare
4279c42 to
040bab0
Compare
f2723c4 to
299ad19
Compare
pyrefly_strict into pyrefly, ty_all into typyrefly_strict into pyrefly, make ty_all passing
pyrefly_strict into pyrefly, make ty_all passingpyrefly_strict into pyrefly, make ty_all and pyrefly_all passing
Dr-Irv
left a comment
There was a problem hiding this comment.
did go through the tests, so those are reviewed.
There is one open comment from previous review - others were all resolved.
| @override | ||
| def nbytes(self) -> int: ... | ||
| @property | ||
| def size(self) -> int: ... |
There was a problem hiding this comment.
I think you can remove size here
| @override | ||
| # pyrefly: ignore[bad-override] | ||
| def isin(self, values: Iterable[Any], level: Level) -> np_1darray_bool: ... |
There was a problem hiding this comment.
Yes, but the code defines isin allowing None as the argument for level here:
https://github.com/pandas-dev/pandas/blob/4e1b1f77dbc700e505c808e8b6bc164abce87955/pandas/core/indexes/multi.py#L4884
So I think we can delete this override and the tests should still work.
| @override | ||
| def set_names( |
There was a problem hiding this comment.
OK - the implementation is just in Index, but tests for MultiIndex being required if Mapping is the argument
|
|
||
| def my_named_func_1(df: pd.DataFrame) -> pd.Series[str]: | ||
| return df["a"] | ||
| return cast("pd.Series[str]", df["a"]) |
There was a problem hiding this comment.
can we add a comment here why we're OK with the cast in the test?
|
|
||
| def sum_mean(x: pd.DataFrame) -> float: | ||
| return x.sum().mean() | ||
| return cast("pd.Series[float] | pd.Series[int]", x.sum()).mean() |
There was a problem hiding this comment.
Not a fan of the cast here. Why should a user be required to do the cast to get this to work?
There was a problem hiding this comment.
ty reveals x.sum() as Series[Any], and Series[Any]().mean() as Unknown. What do you think? We can make Series[Any]().mean() giving Any.
There was a problem hiding this comment.
At least for pyright, x.sum().mean() is revealed as float there. So check the other type checkers, as this might be a bug in ty
| return Series(val) | ||
|
|
||
| def s2scalar(val: Series) -> float: | ||
| def s2scalar(val: Series[int] | Series[float]) -> float: |
There was a problem hiding this comment.
Multiple places here (I didn't comment on all of them), but we want functions declared as:
def foo(val: Series) -> float:to be acceptable usage. We can't force people to use the generic declarations like Series[int] and Series[float].
Is there a way to keep the tests as they were?
There was a problem hiding this comment.
same comment here as in test_groupby.py about the function declarations
There was a problem hiding this comment.
DatetimeIndexResampler is not documented, so I don't think we should use it in tests. Also, have functions with Series[int] in declaration, but we can't expect users to do that.
There was a problem hiding this comment.
Based on the asserts, I think DatetimeIndexResampler is indeed needed for typing purposes 😕 #848
There was a problem hiding this comment.
I think those assert should be removed.
But I also think that in pandas, DatetimeIndexResampler, PeriodIndexResampler and TimedeltaIndexResampler should be added to the pandas.api.typing module since pandas.api.typing.DataFrameGroupBy.resample can return those types.
There was a problem hiding this comment.
same comment about having functions with Series[int] | Series[float] as argument types
mypy,pyreflyandty#1836 (Replace xxxx with the Github issue number)assert_type()to assert the type of any return value)AGENTS.md.